Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Source Google Analytics Data API: ability to add multiple property ids #30152

Conversation

roman-yermilov-gl
Copy link
Contributor

@roman-yermilov-gl roman-yermilov-gl commented Sep 5, 2023

What

#24739

How

Property ID is now a list. Set of streams is generated based on each property id

@github-actions
Copy link
Contributor

github-actions bot commented Sep 5, 2023

Before Merging a Connector Pull Request

Wow! What a great pull request you have here! 🎉

To merge this PR, ensure the following has been done/considered for each connector added or updated:

  • PR name follows PR naming conventions
  • Breaking changes are considered. If a Breaking Change is being introduced, ensure an Airbyte engineer has created a Breaking Change Plan.
  • Connector version has been incremented in the Dockerfile and metadata.yaml according to our Semantic Versioning for Connectors guidelines
  • You've updated the connector's metadata.yaml file any other relevant changes, including a breakingChanges entry for major version bumps. See metadata.yaml docs
  • Secrets in the connector's spec are annotated with airbyte_secret
  • All documentation files are up to date. (README.md, bootstrap.md, docs.md, etc...)
  • Changelog updated in docs/integrations/<source or destination>/<name>.md with an entry for the new version. See changelog example
  • Migration guide updated in docs/integrations/<source or destination>/<name>-migrations.md with an entry for the new version, if the version is a breaking change. See migration guide example
  • If set, you've ensured the icon is present in the platform-internal repo. (Docs)

If the checklist is complete, but the CI check is failing,

  1. Check for hidden checklists in your PR description

  2. Toggle the github label checklist-action-run on/off to re-run the checklist CI.

@roman-yermilov-gl roman-yermilov-gl force-pushed the ryermilov/source-google-analytics-data-api-multiple-property-id branch from 1b198b1 to e0e2003 Compare September 5, 2023 11:57
@octavia-squidington-iii octavia-squidington-iii added the area/documentation Improvements or additions to documentation label Sep 5, 2023
@roman-yermilov-gl roman-yermilov-gl force-pushed the ryermilov/source-google-analytics-data-api-multiple-property-id branch from 7858762 to cc5db93 Compare September 5, 2023 17:09
@roman-yermilov-gl roman-yermilov-gl marked this pull request as ready for review September 5, 2023 17:56
@roman-yermilov-gl roman-yermilov-gl force-pushed the ryermilov/source-google-analytics-data-api-multiple-property-id branch 2 times, most recently from 582e75a to 1975405 Compare September 6, 2023 20:30
@airbytehq airbytehq deleted a comment from github-actions bot Sep 6, 2023
@airbytehq airbytehq deleted a comment from github-actions bot Sep 6, 2023
@airbytehq airbytehq deleted a comment from github-actions bot Sep 6, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Sep 6, 2023

source-google-analytics-data-api test report (commit 19754052f4) - ✅

⏲️ Total pipeline duration: 06mn47s

Step Result
Connector package install
Build source-google-analytics-data-api docker image for platform linux/x86_64
Unit tests
Acceptance tests
Code format checks
Validate airbyte-integrations/connectors/source-google-analytics-data-api/metadata.yaml
Connector version semver check
Connector version increment check
QA checks

🔗 View the logs here

☁️ View runs for commit in Dagger Cloud

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-google-analytics-data-api test

@@ -43,7 +43,7 @@ def patch_base_class():
@pytest.fixture
def config():
return {
"property_id": "108176369",
"property_ids": ["108176369"],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add unit test with old config and try test backward compatibility in that way

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in scope of migration

@@ -4,7 +4,7 @@
"$schema": "https://json-schema.org/draft-07/schema#",
"title": "Google Analytics (Data API) Spec",
"type": "object",
"required": ["property_id", "date_ranges_start_date"],
"required": ["property_ids", "date_ranges_start_date"],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you test those changes manually for backward compatibility?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did, but it seems I missed something. Now migration is prepared and everithing is tested


for report in _config["custom_reports"]:
invalid_dimensions = set(report["dimensions"]) - dimensions
if invalid_dimensions:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add spaces between condition blocks and add comments for them

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

raise wrong_property_id_error

if not metadata:
return False, "failed to get metadata, over quota, try later"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error message better start with upper case letter

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -39,6 +39,7 @@ def patch_base_class(mocker):

return {
"config": {
"property_ids": ["496180525"],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if an old config has not property_ids

what happens next?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Figured out in scope of migration

@github-actions
Copy link
Contributor

source-google-analytics-data-api test report (commit c4573aca3d) - ✅

⏲️ Total pipeline duration: 10mn35s

Step Result
Connector package install
Build source-google-analytics-data-api docker image for platform linux/x86_64
Unit tests
Acceptance tests
Code format checks
Validate airbyte-integrations/connectors/source-google-analytics-data-api/metadata.yaml
Connector version semver check
Connector version increment check
QA checks

🔗 View the logs here

☁️ View runs for commit in Dagger Cloud

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-google-analytics-data-api test

@roman-yermilov-gl roman-yermilov-gl force-pushed the ryermilov/source-google-analytics-data-api-multiple-property-id branch from 9e0dc49 to a3a4f1f Compare September 13, 2023 15:25
@roman-yermilov-gl roman-yermilov-gl force-pushed the ryermilov/source-google-analytics-data-api-multiple-property-id branch from 4bfe2e9 to 815f1ed Compare September 13, 2023 16:07
@github-actions
Copy link
Contributor

source-google-analytics-data-api test report (commit a3a4f1f912) - ❌

⏲️ Total pipeline duration: 46mn31s

Step Result
Connector package install
Build source-google-analytics-data-api docker image for platform linux/x86_64
Unit tests
Acceptance tests
Code format checks
Validate airbyte-integrations/connectors/source-google-analytics-data-api/metadata.yaml
Connector version semver check
Connector version increment check
QA checks

🔗 View the logs here

☁️ View runs for commit in Dagger Cloud

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-google-analytics-data-api test

@github-actions
Copy link
Contributor

source-google-analytics-data-api test report (commit 815f1ed462) - ✅

⏲️ Total pipeline duration: 51mn35s

Step Result
Connector package install
Build source-google-analytics-data-api docker image for platform linux/x86_64
Unit tests
Acceptance tests
Code format checks
Validate airbyte-integrations/connectors/source-google-analytics-data-api/metadata.yaml
Connector version semver check
Connector version increment check
QA checks

🔗 View the logs here

☁️ View runs for commit in Dagger Cloud

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-google-analytics-data-api test

Copy link
Collaborator

@lazebnyi lazebnyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please run tests with old format config

@roman-yermilov-gl
Copy link
Contributor Author

Please run tests with old format config

The tests were run with old config. If format is new, it means migration was applied during CI run

@roman-yermilov-gl roman-yermilov-gl merged commit 6442bd1 into master Sep 14, 2023
26 of 27 checks passed
@roman-yermilov-gl roman-yermilov-gl deleted the ryermilov/source-google-analytics-data-api-multiple-property-id branch September 14, 2023 11:12
@Talalqaddoumi
Copy link

I recommend consolidating the data into the same stream rather than creating a stream per table per property. The latter approach proves unhelpful and leads to complications when refreshing the source schema, especially after the addition of numerous properties. Currently, I'm facing difficulties generating and refreshing the source schema in a setup with 160 properties.

Here is an illustration of the challenge:
image

Furthermore, adopting this approach would necessitate additional transformations in the destination to append the data into a single table.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation connectors/source/google-analytics-data-api
Projects
None yet
4 participants